fix(wallet): return error instead of panicking on invalid current_height#449
Open
none34829 wants to merge 1 commit intobitcoindevkit:masterfrom
Open
fix(wallet): return error instead of panicking on invalid current_height#449none34829 wants to merge 1 commit intobitcoindevkit:masterfrom
none34829 wants to merge 1 commit intobitcoindevkit:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #449 +/- ##
=======================================
Coverage 80.21% 80.21%
=======================================
Files 24 24
Lines 5348 5348
Branches 242 242
=======================================
Hits 4290 4290
Misses 980 980
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
none34829
force-pushed
the
fix/invalid-current-height
branch
from
4065bc8 to
b8a0b5e
Compare
Contributor
Author
|
Thanks for the review- the type-safe approach is definitely cleaner.
|
3 tasks
Contributor
Author
|
CI failures here are unrelated to this PR- I've opened #455 with a focused mechanical rename to |
ValuedMammal
added a commit
that referenced
this pull request
Apr 23, 2026
…_unchecked be710a8 chore(tests): replace deprecated FeeRate::from_sat_per_vb_unchecked (none34829) Pull request description: ### Description `bitcoin-units 0.1.3` deprecated `FeeRate::from_sat_per_vb_unchecked` in favor of `FeeRate::from_sat_per_vb_u32`. Cargo resolves any 0.1.x automatically, so fresh CI runs started failing on master (and all open PRs) once 0.1.3 was published. This patch swaps every call site over to the new name. The two functions compute the same value for any input that fits in `u32`; every existing call site passes a small literal (1, 3, 5, 10, 25, 50, 255, 454, 1000, 10_000), so no behavioural change. All usages are in test/fixture code (`src/wallet/coin_selection.rs` test module, `tests/wallet.rs`, `tests/build_fee_bump.rs`) — no production code path was using the deprecated function. ### Notes to the reviewers - Pure mechanical rename; no logic or type changes. - Unblocks CI on master and every open PR (e.g. #449, #442, #445, #448). ### Changelog notice N/A — internal test-only change. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `just p` before pushing ACKs for top commit: ValuedMammal: ACK be710a8 Tree-SHA512: b2354fcf6e39228bc5796f92af4d3692fbdb750267c2c5a7d814fc5c84f4af64b746562fda7e01dac282ba04acb17ff7bf5c7ef0155a3d6f302974003baf0629
none34829
force-pushed
the
fix/invalid-current-height
branch
from
b8a0b5e to
6dac1a0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #49.
TxBuilder::current_heightand the fallback tochain.tip().height()increate_txboth calledabsolute::LockTime::from_height(h).expect(...), which panics wheneverh >= 500_000_000. The Wizardsardine audit flagged this because it lets a remote Electrum/Esplora server crash the wallet by reporting a tip height at or above that threshold — panics on externally-provided input are unsafe for a library.This PR replaces the panics with a typed error:
TxParams.current_heightnow stores a rawu32instead of an already-validatedabsolute::LockTime, so the builder setter no longer has to validate in a position where it can't return aResult.Wallet::create_txperforms theu32 -> LockTimeconversion in one place, mapping theConversionErrorto a newCreateTxError::InvalidCurrentHeight(u32)variant via?.TxBuilder::current_height) and the implicit-fallback path (chain tip) now flow through the same validation and surface the same error.Notes to the reviewers
TxParams.current_heightis crate-private (pub(crate)) so there's no external API break there. TheTxBuilder::current_heightpublic signature is unchanged.CreateTxErroris technically an additive change, but it is an enum so downstream exhaustive matches would need to be updated.CreateTxErroris not marked#[non_exhaustive], which is tracked separately in Consider making all error enum variants#[non_exhaustive]#239.Changelog notice
Wallet::create_txnow returnsCreateTxError::InvalidCurrentHeightinstead of panicking whenTxBuilder::current_heightor the fallback chain tip height is>= 500_000_000.Checklists
All Submissions:
just pbefore pushing (140 lib tests + 118 wallet + 22 fee_bump + 10 persisted + 13 descriptor_macro + 54 doctests all pass; clippy clean; fmt clean; cargo doc clean)Bugfixes: